Mark the current page with aria-current in both navs - #2471
Mark the current page with aria-current in both navs#2471opensource-joe wants to merge 1 commit into
Conversation
Closes pydata#1886. The sidebar and header nav both know which page you are on, and both say so only with a `current` CSS class. A class is invisible to assistive technology, so a screen reader user gets no "you are here" in either nav. The breadcrumb already uses `aria-current="page"`; this gives the two navigations the same treatment. The scope is the narrower of the two things the class does. `current` highlights the whole section you are inside, which is right for a visual highlight and wrong for `aria-current="page"`, so the attribute goes only on the page actually being viewed. In the header nav that means a top-level entry is marked when you are on that page and not when you are merely somewhere beneath it, which is what was agreed on the issue. That leaves the second half of the issue open on purpose. Conveying "you are somewhere in this section" is a real question, and `page` is not the answer to it. There is more on that in the pull request. The sidebar needed the same handling on the cached path. `_move_current_markers` reuses a sibling page's rendered sidebar and moves the `current` markers onto this page's entry, so `aria-current` has to move with them. A cached sidebar that kept the attribute would announce the wrong page as current, which is worse than marking nothing. `test_sidebar_toctree_cache` catches exactly this: it asserts a cached sidebar is byte-identical to a freshly built one, and it failed until the attribute moved too. Three regression fixtures pick up the new attribute. The added test states the intent directly rather than relying on those snapshots: exactly one sidebar entry is the current page, ancestors are not, and the header nav marks a section only on its own landing page.
|
Sounds like AI to me, both the PR description and the code. Author has 200 contributions in two weeks... I am not saying it's not useful because this issue was clearly forgotten. About the code though, why does Claude feel the urge to write 3 to 5 line comments above every other line of code? |
|
@Yann-P, I use Claude to assist me, as noted on my profile; however, I've been doing open source, a11y, etc., for a while and am happy to help if you'd like. I recognize that I am responsible for my own work even when using AI assistance. Some repos have an AI policy in their contributing guidelines. |
Yann-P
added
the
tag: accessibility
|
@Yann-P @opensource-joe brings up a good point. I'm the documentation lead of icalendar, and our team has struggled with contributors who misuse AI. Additionally, the agency that funds much of work has a fairly strict AI policy. Maybe this should be a new issue or a discussion topic? Here are some references.
Side note: I 😍 this theme, and appreciate all the work that's gone into this project. |
|
Thank you Steve for these resources! Theses AI contributions are useful especially when it comes to accessibility but at the same time it's very unpleasant to be thrown a big wall of text straight out of Claude. |
|
Manual testing on https://pydata-sphinx-theme--2471.org.readthedocs.build/en/2471/index.html: works as expected per #1886. Thank you |
|
wondering if is_current_page should not be called is_aria_current_page since all the logic for is_current_page revolves around the desired aria behavior. But no strong opinion |
@Yann-P, this is true, and I appreciate the feedback as I continue to have an agent assist. Hope the PR was helpful overall. |
3 participants
Closes #1886.
Both navs already know which page you are on, and both say so only with a
currentCSS class. That is invisible to assistive technology, so a screen reader user gets no "you are here" in either navigation. The breadcrumb has usedaria-current="page"for a while; this gives the sidebar and header nav the same treatment.Scope, and the half I have left open
currentdoes two jobs. It highlights the whole section you are inside, which is right for a visual highlight, and it marks the page you are on. Only the second is whataria-current="page"means, so the attribute goes only on the page actually being viewed.For the header nav that matches what @drammock asked for on the issue: a top-level entry is marked when you are on that page, and not when you are merely somewhere beneath it.
user_guide/ablog.html, inside a sectionuser_guide/index.html, the section's own page@gabalafou's other question is deliberately still open. You asked how best to convey the thing the underline and the notch convey visually, that the reader is somewhere inside this section. I do not think
aria-current="page"is that answer, and I did not want to quietly decide it inside a bug fix.The candidate worth discussing is
aria-current="location", which is a valid token and is described as the current location within a context rather than the current page. If you like it, it is a small follow-up on top of this, andis_currentalready carries exactly the state it needs. Happy to write it either way, but it is your call to make rather than mine.The cached sidebar needed the same handling
This was the interesting part.
_move_current_markersreuses a sibling page's rendered sidebar and relocates thecurrentmarkers onto this page's entry, soaria-currenthas to move with them. A cached sidebar that kept the attribute would announce the wrong page as the current one, which is worse than marking nothing at all.test_sidebar_toctree_cachecatches this precisely, since it asserts a cached sidebar is byte-identical to a freshly built one. It failed until the attribute moved too, which is a good test.Verification
tox -e py312-tests-no-cov, 116 passedtox -e a11y-tests-chromium, 34 passed, 2 xfailedtox -e docs-dev, and I read the rendered HTML rather than trusting the source: on a nested pagearia-current="page"appears twice, breadcrumb and sidebar, and on a section landing page three times, with the header nav includedruff checkandruff formatcleanThree regression fixtures pick up the new attribute, and the diff in each is one line. The added test states the intent directly rather than leaning on those snapshots, and it fails without the change with
assert 0 == 1on "exactly one sidebar entry may be the current page".One note on the environment, in case it helps anyone else:
tox -e docs-devexits 1 on a missing graphvizdotbinary after writing complete HTML, so that exit code is not a failed build.